Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove extraneous whitespace - #26275 #26285

Merged
merged 18 commits into from Feb 7, 2020
Merged

Remove extraneous whitespace - #26275 #26285

merged 18 commits into from Feb 7, 2020

Conversation

DanielRuf
Copy link
Contributor

@DanielRuf DanielRuf commented Jan 7, 2020 •
edited
Loading

Description (*)

This PR removes the extraneous spaces from the labels.

Fixed Issues (if relevant)

  1. Whitespace between label and required star on Checkout page #26275

Manual testing scenarios (*)

  1. Go to checkout
  2. There should not be any extra whitespace before the asterisk / required icon.

Without this PR:
grafik

With the fixes in the PR:
grafik

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Jan 7, 2020

Hi @DanielRuf. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@DanielRuf DanielRuf mentioned this pull request Jan 7, 2020
Closed
@DanielRuf
Copy link
Contributor Author

Which violations are meant here?

https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/26285/d17f81ec84bfc2dbd18f5b8133fd880c/Statics/allure-report-ce/index.html#categories/78ae151bd5d3aa3d8f50dc36ec4ec082/abecdf50b283f5ae/

PHP Code Sniffer detected 2 violation(s): 

FILE: ...code/Magento/Customer/view/frontend/templates/address/edit.phtml
----------------------------------------------------------------------
FOUND 0 ERRORS AND 34 WARNINGS AFFECTING 24 LINES
----------------------------------------------------------------------
  25 | WARNING | [x] Expected 0 space(s) after closing parenthesis;
     |         |     found 1
  29 | WARNING | [x] Expected 0 space(s) after closing parenthesis;
     |         |     found 1
  33 | WARNING | [x] Expected 0 space(s) after closing parenthesis;
     |         |     found 1
  40 | WARNING | [ ] The use of helpers in templates is discouraged.
     |         |     Use ViewModel instead.
  40 | WARNING | [ ] Line exceeds 120 characters; contains 137
     |         |     characters
  42 | WARNING | [ ] Line exceeds 120 characters; contains 146
     |         |     characters
  52 | WARNING | [ ] The use of helpers in templates is discouraged.
     |         |     Use ViewModel instead.
  52 | WARNING | [x] Expected 0 space(s) after closing parenthesis;
     |         |     found 1
  52 | WARNING | [ ] Line exceeds 120 characters; contains 142
     |         |     characters
  70 | WARNING | [ ] The use of helpers in templates is discouraged.
     |         |     Use ViewModel instead.
  70 | WARNING | [x] Expected 0 space(s) after closing parenthesis;
     |         |     found 1
  80 | WARNING | [ ] The use of helpers in templates is discouraged.
     |         |     Use ViewModel instead.
  80 | WARNING | [ ] Line exceeds 120 characters; contains 170
     |         |     characters
  86 | WARNING | [ ] Line exceeds 120 characters; contains 140
     |         |     characters
  92 | WARNING | [ ] The use of helpers in templates is discouraged.
     |         |     Use ViewModel instead.
  92 | WARNING | [ ] Line exceeds 120 characters; contains 164
     |         |     characters
 103 | WARNING | [ ] Line exceeds 120 characters; contains 160
     |         |     characters
 104 | WARNING | [ ] Line exceeds 120 characters; contains 121
     |         |     characters
 111 | WARNING | [ ] The use of helpers in templates is discouraged.
     |         |     Use ViewModel instead.
 111 | WARNING | [ ] Line exceeds 120 characters; contains 279
     |         |     characters
 124 | WARNING | [ ] The use of helpers in templates is discouraged.
     |         |     Use ViewModel instead.
 124 | WARNING | [ ] Line exceeds 120 characters; contains 196
     |         |     characters
 131 | WARNING | [ ] Line exceeds 120 characters; contains 149
     |         |     characters
 137 | WARNING | [x] Expected 0 space(s) after closing parenthesis;
     |         |     found 1
 141 | WARNING | [x] Expected 0 space(s) after closing parenthesis;
     |         |     found 1
 148 | WARNING | [x] Expected 0 space(s) after ELSE keyword; 1 found
 152 | WARNING | [x] Expected 0 space(s) after closing parenthesis;
     |         |     found 1
 156 | WARNING | [x] Expected 0 space(s) after closing parenthesis;
     |         |     found 1
 163 | WARNING | [x] Expected 0 space(s) after ELSE keyword; 1 found
 192 | WARNING | [ ] Line exceeds 120 characters; contains 131
     |         |     characters
 197 | WARNING | [ ] The use of helpers in templates is discouraged.
     |         |     Use ViewModel instead.
 197 | WARNING | [ ] Line exceeds 120 characters; contains 123
     |         |     characters
 199 | WARNING | [ ] The use of helpers in templates is discouraged.
     |         |     Use ViewModel instead.
 199 | WARNING | [ ] Line exceeds 120 characters; contains 154
     |         |     characters
----------------------------------------------------------------------
PHPCBF CAN FIX THE 11 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------



Failed asserting that 2 matches expected 0.

@marcoaacoliveira
Copy link
Member

@DanielRuf Thanks for your contribution. You changed a file and when you do it Magento will run static tests against this file, so even if the problem is older than your changes you must fix all it to get pass in the tests.

Specifically in your case changes aren't hard...

You must do 2 types of refactoring:

First:

You must change all $this->helper references for viewModel approach, which is basically:

  • Instead of using $this->helper, you must create a new block viewModel (if it doesn't exists), and it must be the responsible for all data or functionality (handling data matters). In DevDocs you can find how to do it step by step.

Second:

The easiest one, you just have to guarantee that PHP code lines aren't too big, it must be equal or lesser than 120 characters per line.

@DanielRuf
Copy link
Contributor Author

Well it says "2 violations" which is not very clear. Also warnings should be actual errors then.

@marcoaacoliveira
Copy link
Member

marcoaacoliveira commented Jan 7, 2020 •
edited
Loading

Indeed, maybe a maintainer can be more helpful to this matter.

@marcoaacoliveira marcoaacoliveira removed their request for review January 7, 2020 11:08
@DanielRuf
Copy link
Contributor Author

Currently trying to fix the outstanding PHPCS violations.

@marcoaacoliveira
Copy link
Member

Good luck and have fun :)

@DanielRuf
Copy link
Contributor Author

Hm, I see no real step for step description for migrating the \Magento\Customer\Helper\Address to a ViewModel. Also the link in the DevDocs at the bottom does not seem to point to the mentioned view_model part.

@DanielRuf
Extract some values and helpers
574f052 
Extract them to shorten lines and resolve codestyle warnings
@DanielRuf
Copy link
Contributor Author

Not sure about the helpers as I have mostly only worked on the frontend part of Magento 2.

Would be great to get some help with this.

@DanielRuf
Copy link
Contributor Author

I will check the failing tests later.

@magento-engcom-team
Copy link
Contributor

Hi @nuzil, thank you for the review.
ENGCOM-6581 has been created to process this Pull Request
✳️ @nuzil, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@nuzil nuzil added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Jan 13, 2020
@engcom-Delta engcom-Delta self-assigned this Jan 15, 2020
@engcom-Delta
Copy link
Contributor

✔️ QA passed

@engcom-Golf engcom-Golf self-assigned this Jan 15, 2020
@engcom-Golf
Copy link
Contributor

@DanielRuf i will take care of satic test

@DanielRuf
Copy link
Contributor Author

@DanielRuf i will take care of satic test

I already did. No need to.

magento-engcom-team pushed a commit that referenced this pull request Feb 7, 2020
@slavvka
ENGCOM-6581: Remove extraneous whitespace - #26275 #26285
0850e5e
@magento-engcom-team magento-engcom-team merged commit 4b9b6fa into magento:2.4-develop Feb 7, 2020
@m2-assistant
Copy link

m2-assistant bot commented Feb 7, 2020

Hi @DanielRuf, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@Bartlomiejsz Bartlomiejsz mentioned this pull request Feb 9, 2020
Open
4 tasks
@DanielRuf DanielRuf deleted the fix/26275-checkout-required-fields-remove-space branch April 4, 2020 18:02
@sdzhepa sdzhepa mentioned this pull request May 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nuzil nuzil nuzil approved these changes

@Nazar65 Nazar65 Nazar65 approved these changes

@melnikovi melnikovi Awaiting requested review from melnikovi

@paliarush paliarush Awaiting requested review from paliarush

Assignees

@nuzil nuzil

@VladimirZaets VladimirZaets

@engcom-Delta engcom-Delta

@engcom-Golf engcom-Golf

Labels
Area: Frontend Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Component: Checkout Component: Customer Component: Ui Progress: accept Release Line: 2.4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@DanielRuf @marcoaacoliveira @nuzil @magento-engcom-team @engcom-Delta @engcom-Golf @Nazar65 @VladimirZaets